Migrate checkstrformat to use ErrorMessage class
#7
Conversation
WalkthroughThis update standardizes and centralizes error message handling for string and bytes formatting errors in the codebase. All explicit error message strings in Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧰 Additional context used🪛 Ruff (0.8.2)mypy/checkstrformat.py904-906: Use a single (SIM102) 🔇 Additional comments (23)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request migrates error codes in checkstrformat.py to use ErrorMessage objects stored in message_registry. This change improves the consistency and maintainability of error messages related to string formatting. The use of predefined message constants makes it easier to update and manage error messages in the future. The code changes are well-structured and easy to follow.
Summary of Findings
- Missing Error Code in
UNRECOGNIZED_FORMAT_SPEC: The error messageUNRECOGNIZED_FORMAT_SPECinmessage_registry.pylacks an associated error code. While the other new error messages havecodes.STRING_FORMATTING, this one is missing it, which could lead to inconsistencies in error reporting and filtering.
Merge Readiness
The pull request is well-structured and improves the consistency of error messages. However, the missing error code for UNRECOGNIZED_FORMAT_SPEC should be addressed before merging. I am unable to directly approve this pull request, and recommend that another reviewer also assesses these changes. After addressing the identified issue, the pull request will be ready for merging.
| ): | ||
| # TODO: add support for some custom specs like datetime? | ||
| self.msg.fail( | ||
| "Unrecognized format" ' specification "{}"'.format(spec.format_spec[1:]), | ||
| message_registry.UNRECOGNIZED_FORMAT_SPEC.format(spec.format_spec[1:]), | ||
| call, | ||
| code=codes.STRING_FORMATTING, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Consider adding a code to the fail call, for consistency with the other calls in this file. This will also allow users to filter this error specifically.
self.msg.fail(
message_registry.UNRECOGNIZED_FORMAT_SPEC.format(spec.format_spec[1:])
call,
code=codes.STRING_FORMATTING,
)
continue
|
/codehelper review |
![devd-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| @@ -316,3 +316,74 @@ def with_additional_msg(self, info: str) -> ErrorMessage: | |||
| ARG_NAME_EXPECTED_STRING_LITERAL: Final = ErrorMessage( | |||
| "Expected string literal for argument name, got {}", codes.SYNTAX | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Improvement: Good refactoring to centralize error messages in message_registry.py. This makes error messages more consistent and easier to maintain.
| @@ -316,3 +316,74 @@ def with_additional_msg(self, info: str) -> ErrorMessage: | |||
| ARG_NAME_EXPECTED_STRING_LITERAL: Final = ErrorMessage( | |||
| "Expected string literal for argument name, got {}", codes.SYNTAX | |||
| ) | |||
|
|
|||
| FORMAT_STR_INVALID_SPECIFIER: Final = ErrorMessage( | |||
There was a problem hiding this comment.
Suggestion: Consider adding a docstring for this new section of error messages to explain that these are all related to string formatting.
| @@ -269,6 +269,7 @@ def span_from_context(ctx: Context) -> Iterable[int]: | |||
| allow_dups=allow_dups, | |||
| ) | |||
|
|
|||
| @overload | |||
There was a problem hiding this comment.
Enhancement: The @overload decorator for fail() method is a good addition that improves type checking when using the new ErrorMessage objects.
| ) -> None: | ||
| ... | ||
|
|
||
| def fail( |
There was a problem hiding this comment.
Improvement: The implementation of fail() now handles both string messages and ErrorMessage objects, which is a good design that maintains backward compatibility.
| ) -> None: | ||
| """Report an error message (unless disabled).""" | ||
| if isinstance(msg, message_registry.ErrorMessage): |
There was a problem hiding this comment.
Style: Consider adding a type annotation for the local variables msg_str to make the code more explicit about types.
| @@ -112,10 +112,10 @@ def __str__(self) -> str: | |||
| VALID_NEWTYPE: Final = ErrorCode( | |||
| "valid-newtype", "Check that argument 2 to NewType is valid", "General" | |||
| ) | |||
| STRING_FORMATTING: Final = ErrorCode( | |||
| STRING_FORMATTING: Final[ErrorCode] = ErrorCode( | |||
There was a problem hiding this comment.
Enhancement: Adding Final[ErrorCode] type annotation provides better type safety than just Final.
| "str-format", "Check that string formatting/interpolation is type-safe", "General" | ||
| ) | ||
| STR_BYTES_PY3: Final = ErrorCode( | ||
| STR_BYTES_PY3: Final[ErrorCode] = ErrorCode( |
There was a problem hiding this comment.
Consistency: Good job applying the same Final[ErrorCode] type annotation to STR_BYTES_PY3 as well for consistency.
A granular reimplementation of python#12004
Changes
checkstrformat.pyto useErrorMessageobjects stored inmessage_registryfailmethod inMessageBuilderto acceptErrorMessageobjects (the overload is similar to the one here).Summary by CodeRabbit
New Features
Refactor
Style